Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extend meta_df tests to include datetime #236

Merged
merged 18 commits into from
May 29, 2019
Merged

Extend meta_df tests to include datetime #236

merged 18 commits into from
May 29, 2019

Conversation

znicholls
Copy link
Collaborator

@znicholls znicholls commented May 14, 2019 •
edited
Loading

Please confirm that this PR has done the following:

  • Tests Added
  • Documentation Added (N/A)
  • Description in RELEASE_NOTES.md Added

Adding to RELEASE_NOTES.md (remove section after adding to RELEASE_NOTES.md)

Please add a single line in the release notes similar to the following:

- (#XX)[http://link-to-pr.com] Added feature which does something

Description of PR

Closes #222. Good news is this was already sorted in v0.2.0 so all this does is make sure it stays that way by expanding the tests a bit.

stickler-ci
stickler-ci reviewed May 14, 2019
View reviewed changes
pyam/core.py Outdated Show resolved Hide resolved
tests/test_cast_to_iamc.py Outdated Show resolved Hide resolved
tests/test_cast_to_iamc.py Outdated Show resolved Hide resolved
tests/test_cast_to_iamc.py Outdated Show resolved Hide resolved
tests/test_cast_to_iamc.py Outdated Show resolved Hide resolved
tests/test_core.py Outdated Show resolved Hide resolved
tests/test_core.py Outdated Show resolved Hide resolved
tests/test_core.py Outdated Show resolved Hide resolved
tests/test_core.py Outdated Show resolved Hide resolved
tests/test_feature_compare.py Outdated Show resolved Hide resolved
@znicholls znicholls changed the title WIP: title TBC Extended meta_df tests to include datetime May 14, 2019
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 92.009% when pulling 6b5688d on znicholls:move-idx-definitions into a73fc6a on IAMconsortium:master.

@coveralls
Copy link

coveralls commented May 14, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 92.022% when pulling 5cbcb42 on znicholls:move-idx-definitions into 25aa7f0 on IAMconsortium:master.

@znicholls znicholls changed the title Extended meta_df tests to include datetime Extend meta_df tests to include datetime May 14, 2019
danielhuppmann
danielhuppmann requested changes May 24, 2019
View reviewed changes
Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @znicolls for extending the tests - important steps towards full support of datetime!

one request to add a sanity check on the new `swap_time_for_year()´ function, and some suggestions for a more concise notation in the tests.

pyam/core.py Show resolved Hide resolved
tests/test_feature_append_rename_convert.py Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/test_cast_to_iamc.py Outdated Show resolved Hide resolved
tests/test_core.py Outdated Show resolved Hide resolved
tests/test_feature_aggregate.py Outdated Show resolved Hide resolved
tests/test_feature_append_rename_convert.py Outdated Show resolved Hide resolved
tests/test_feature_compare.py Outdated Show resolved Hide resolved
znicholls and others added 4 commits May 24, 2019 20:15
@znicholls @danielhuppmann
Add failing test of sanity check
26a790a 
Co-authored-by: danielhuppmann <huppmann@iiasa.ac.at>
@znicholls @danielhuppmann
Pass sanity check test
9cf2369 
Co-authored-by: danielhuppmann <huppmann@iiasa.ac.at>
@znicholls @danielhuppmann
Fix up double quotes
201b3fa 
Co-authored-by: danielhuppmann <huppmann@iiasa.ac.at>
@znicholls @danielhuppmann
Put dts into conftest
665436f 
Co-authored-by: danielhuppmann <huppmann@iiasa.ac.at>
stickler-ci
stickler-ci reviewed May 24, 2019
View reviewed changes
tests/conftest.py Show resolved Hide resolved
tests/test_cast_to_iamc.py Outdated Show resolved Hide resolved
tests/test_feature_aggregate.py Outdated Show resolved Hide resolved
@znicholls
Copy link
Collaborator Author

cheers @danielhuppmann, that should all be addressed now!

@znicholls
Copy link
Collaborator Author

not sure what's going on with CI, read through pyproj4/pyproj#134 but am none the wiser...

@danielhuppmann
Copy link
Member

sorry about this - @gidden and I already discussed that the region-plotting-tests are quite unstable. How about deactivating those for Windows machines?

@gidden
Copy link
Member

gidden commented May 28, 2019

Hi folks - indeed these are our most finicky tests. One suggestion before turning them off for windows.

In pyproj4/pyproj#134, someone notes that the issue goes away for pyproj>2.1.

On appveyor, I see that the version is:

pyproj: 1.9.6

So perhaps we can try adding the minimum version to our appveyor install?

If that still fails, then I'm ok with just turning them off on windows..

@gidden
Copy link
Member

gidden commented May 29, 2019

Hey @znicholls, I'm now digging through more geopandas/cartopy/pyproj issues. I have two thoughts.

First is related to geopandas/geopandas#830, which suggests a specific pyproj version for windows. To attempt this, can you please revert the update in ci/environment-conda-forge.txt and try pinning only the appveyor install to "pyproj=1.9.4"?

Second, I'm looking through our last successful CI on windows: https://ci.appveyor.com/project/gidden/pyam/builds/24523793/job/2idsdm0hhgvg2tfw

In reviewing the package versions at runtime, there is no difference between that run and 60ccc9a. In reviewing the installed conda packages, however, I see the following difference:

Successful run:

proj4                     5.2.0                ha925a31_1

Failed run:

proj4                     5.2.0             h6538335_1002    conda-forge

Thus this implies one potential fix is to install proj4 via the first conda install (without -c conda-forge).

I know this is annoying, but if you would be willing to try those two changes, that would be amazing. Otherwise, let's just turn this stuff off for windows.

@gidden
Copy link
Member

gidden commented May 29, 2019

I don't know whether to be happy or sad, but it looks fixed now!

gidden
gidden reviewed May 29, 2019
View reviewed changes
pyam/core.py Outdated Show resolved Hide resolved
gidden
gidden reviewed May 29, 2019
View reviewed changes
pyam/core.py Outdated Show resolved Hide resolved
@gidden
Copy link
Member

gidden commented May 29, 2019

A few inline comments, then this is g2g for me. @danielhuppmann you still have this blocking in review, so let us know if it passes muster!

@znicholls @gidden
Update with comment from @gidden
5cbcb42 
Co-authored-by: gidden <matthew.gidden@gmail.com>
@gidden
Copy link
Member

gidden commented May 29, 2019

Looks great! I'll let @danielhuppmann pull this one in.

danielhuppmann
danielhuppmann approved these changes May 29, 2019
View reviewed changes
Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @znicholls

@danielhuppmann danielhuppmann merged commit 1d60716 into IAMconsortium:master May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gidden gidden gidden left review comments

@stickler-ci stickler-ci stickler-ci left review comments

@danielhuppmann danielhuppmann danielhuppmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Aggregate fails with time column
5 participants
@znicholls @coveralls @danielhuppmann @gidden @stickler-ci